[wip] feat: state function changes#6
Closed
merklefruit wants to merge 15 commits intoop-rs:clabby/goerli-genesisfrom
merklefruit:merklefruit/state-function
Closed
[wip] feat: state function changes#6merklefruit wants to merge 15 commits intoop-rs:clabby/goerli-genesisfrom merklefruit:merklefruit/state-function
merklefruit wants to merge 15 commits intoop-rs:clabby/goerli-genesisfrom
merklefruit:merklefruit/state-function
Conversation
merklefruit
changed the title
merklefruit
commented
Jun 24, 2023
| } | ||
|
|
||
| let new_sender_info = to_reth_acc(&sender_account.info); | ||
| post_state.change_account(sender, old_sender_info, new_sender_info); |
Author
There was a problem hiding this comment.
Potential problem: when changing the post_state before the tx execution like we do here, does this create a clash with the state object returned by the execution? hopefully not, but have to test this
Comment on lines
+486
to
+501
| // Route the l1 cost and base fee to the appropriate optimism vaults | ||
| self.increment_account_balance( | ||
| optimism::l1_cost_recipient(), | ||
| l1_cost, | ||
| &mut post_state, | ||
| )?; | ||
| self.increment_account_balance( | ||
| optimism::base_fee_recipient(), | ||
| U256::from( | ||
| block | ||
| .base_fee_per_gas | ||
| .unwrap_or_default() | ||
| .saturating_mul(result.gas_used()), | ||
| ), | ||
| &mut post_state, | ||
| )?; |
Author
There was a problem hiding this comment.
can probably be refactored to be prettier
merklefruit
commented
Jun 24, 2023
Comment on lines
+416
to
+421
| if let Some(m) = transaction.mint() { | ||
| // Add balance to the caller account equal to the minted amount. | ||
| // Note: this is unconditional, and will not be reverted if the tx fails | ||
| // (unless the block can't be built at all due to gas limit constraints) | ||
| sender_account.info.balance += U256::from(m); | ||
| } |
Author
There was a problem hiding this comment.
This could have been addressed in Revm's transact() function in a more clean way. In fact I have a local branch of what this change would look like here: https://github.com/merklefruit/revm/pull/1/files#diff-1d478ba44ccc56e3b1142bd3723bf97f3e254c25dd18323481aedadce0803e91R130
However that solution has some cons:
- we are not able to revert the deposit if the block runs out of gas, because revm's transact() context doesn't know about the outstanding block gas used (called
cumulative_gas_usedhere) - we can't cache the values to calculate
l1_costfor the entire block, and would have to access the DB for each transaction in the block instead. This is the main reason why I wanted to keep all the diffs on Op-reth if possible
Closed
Author
|
Changes rebased in #8 |
Closed
clabby
added a commit
that referenced
this pull request
Aug 12, 2023
clabby
added a commit
that referenced
this pull request
Aug 13, 2023
Resolution checkpoint Resolution checkpoint #2 Resolution checkpoint #3 x Resolution checkpoint #4 Resolution checkpoint #5 Resolution checkpoint #6 Resolution checkpoint #7 Resolution checkpoint #8 Resolve checkpoint #9 (transaction primitive) Resolve checkpoint #10 (rpc api transactions) Resolve checkpoint #11 (building w/o feature flag) Start review Compiling with and without `optimism` feature flag Remove `DepositTx` from txpool mock tests, they never go into the txpool fmt code lint fix signature tests Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com> Use free CI runners (revert before upstream) Co-authored-by: refcell <abigger87@gmail.com> Signature test fixes Co-authored-by refcell <abigger87@gmail.com> Fix Receipt proptest Co-authored-by BB <brian.t.bland@gmail.com> lint Fix variable-length compact for txtype/transaction Co-authored-by: Brian Bland <brian.t.bland@gmail.com> Fix basefee tests Remove unnecessary rpc deps Co-authored-by: Brian Bland <brian.t.bland@gmail.com> Co-authored-by: refcell <abigger87@gmail.com> Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com> Co-authored-by: Roberto <bayardo@alum.mit.edu>
clabby
added a commit
that referenced
this pull request
Aug 13, 2023
Resolution checkpoint Resolution checkpoint #2 Resolution checkpoint #3 x Resolution checkpoint #4 Resolution checkpoint #5 Resolution checkpoint #6 Resolution checkpoint #7 Resolution checkpoint #8 Resolve checkpoint #9 (transaction primitive) Resolve checkpoint #10 (rpc api transactions) Resolve checkpoint #11 (building w/o feature flag) Start review Compiling with and without `optimism` feature flag Remove `DepositTx` from txpool mock tests, they never go into the txpool fmt code lint fix signature tests Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com> Use free CI runners (revert before upstream) Co-authored-by: refcell <abigger87@gmail.com> Signature test fixes Co-authored-by refcell <abigger87@gmail.com> Fix Receipt proptest Co-authored-by BB <brian.t.bland@gmail.com> lint Fix variable-length compact for txtype/transaction Co-authored-by: Brian Bland <brian.t.bland@gmail.com> Fix basefee tests Remove unnecessary rpc deps Co-authored-by: Brian Bland <brian.t.bland@gmail.com> Co-authored-by: refcell <abigger87@gmail.com> Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com> Co-authored-by: Roberto <bayardo@alum.mit.edu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work in progress: